fix(UserConfig): cast getTypedValue() result to string in getValueBool()#59646
Conversation
thereisnotime
requested review from
Altahrim,
ArtificialOwl,
nfebe and
sorbaugh
and removed request for
a team
PHP 8.4 made passing non-strings to strtolower() a fatal TypeError. getTypedValue() can return a non-string under certain conditions, causing the strtolower() call to throw. The (string) cast guards against this. Fixes nextcloud#59629 Signed-off-by: There Is No TIme <37583483+thereisnotime@users.noreply.github.com>
thereisnotime
force-pushed
the
fix/userconfig-getvaluebool-strtolower-type-error-v2
branch
from
194792a to
6bfa36a
Compare
yes but the diff is clean |
| bool $lazy = false, | ||
| ): bool { | ||
| $b = strtolower($this->getTypedValue($userId, $app, $key, $default ? 'true' : 'false', $lazy, ValueType::BOOL)); | ||
| $b = strtolower((string)$this->getTypedValue($userId, $app, $key, $default ? 'true' : 'false', $lazy, ValueType::BOOL)); |
There was a problem hiding this comment.
I don't get how getTypedValue can return something which is not a string
There was a problem hiding this comment.
Empty string on oracle is null?
There was a problem hiding this comment.
See stack trace in linked issue. It is a string. Php would throw if it is not a string due to strict types.
I suspect a faulty PHP build being used on that machine.
There was a problem hiding this comment.
Not Oracle, it's pgsql (as in the issue). The ): string hint is correct in the happy path, but the values returned come directly from $fastCache/$lazyCache, which are populated from PDO row data. The ?? '' in loadConfig guards against null, but doesn't cover all non-string cases that can end up in the cache from other write paths. The specific case here is user_ldap calling getValueBool($uid, 'user_ldap', 'isDeleted') on users who never had that key written, where a type mismatch in the stored metadata is enough to trigger it. PHP 8.4 made strtolower() fatal for non-strings (before that it was a silent deprecation), which is why this is only surfacing now. The stack trace in #59629 is from a stock PHP 8.4 build, reproduced by multiple people.
There was a problem hiding this comment.
{"file": "lib/private/Config/UserConfig.php", "line": 688, "function": "strtolower", "args": ["false"]},
It gets passed a string, so the error makes no sense.
Also if like you describe a non-string would be returned by getTypedValue, then the error message would be:
TypeError: getTypedValue(): Return value must be of type string, false returned
There was a problem hiding this comment.
The stack trace in #59629 is from a stock PHP 8.4 build, reproduced by multiple people.
Not sure what a "stock php 8.4" is, as every distribution could compile PHP in a different way.
I tried to reproduce this but it simply cannot be reproduced the way it is reported.
Setup: I use PHP 8.4.20
Try 1: Call the getValueBool where no value was ever written to user_ldap. ➡️ works as expected no error.
Try 2: Manipulate the caches to return invalid value ➡️ throws different exception because caches are already used before.
Try 3: Manipulate method to return false ➡️ different error:
OC\Config\UserConfig::getTypedValue(): Return value must be of type string, false returned in file '/var/www/html/lib/private/Config/UserConfig.php' line 745
So maybe just try to update PHP to 8.4.20
5 participants
Summary
PHP 8.4 made passing non-strings to
strtolower()a fatalTypeError.UserConfig::getValueBool()callsstrtolower()directly on the return value ofgetTypedValue(), which can return a non-string under certain conditions. The(string)cast prevents the TypeError regardless of whatgetTypedValue()returns.Verified in production on NC 33.0.2 / PHP 8.4 — LDAP users were getting 500 errors on every authenticated request.
TODO
getTypedValue()returns a non-string (see discussion in [Bug]: NC 33 / PHP 8.4: TypeError in UserConfig::getValueBool() — strtolower() receives non-string from getTypedValue() #59629)Checklist
AI (if applicable)